Skip to content

Add GH16805 RNTupleProcessor regression test#22644

Open
matildamarjamaki wants to merge 3 commits into
root-project:masterfrom
matildamarjamaki:gh16805-rntuple-processor-test
Open

Add GH16805 RNTupleProcessor regression test#22644
matildamarjamaki wants to merge 3 commits into
root-project:masterfrom
matildamarjamaki:gh16805-rntuple-processor-test

Conversation

@matildamarjamaki

Copy link
Copy Markdown

Summary

Adds a regression test for the GH16805 RNTupleProcessor use case.

The test creates several RNTuples corresponding to the original TTree setup ("stepzero", "stepone" and "topLevelFriend"), composes them using "RNTupleProcessor::CreateChain" and "RNTupleProcessor::CreateJoin" and validates the resulting values with "EXPECT_EQ".

The goal is to provide a simple regression test covering a TTree-friends-like workflow implemented with RNTupleProcessor.

@vepadulano vepadulano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @matildamarjamaki for this contribution, I believe it's a good addition! I left a couple of minor remarks but the changes look already good.

Comment thread tree/ntuple/test/CMakeLists.txt Outdated
Comment thread tree/ntuple/test/ntuple_processor_gh16805.cxx Outdated
Comment thread tree/ntuple/test/ntuple_processor_gh16805.cxx Outdated
@github-actions

Copy link
Copy Markdown

Test Results

    21 files      21 suites   3d 12h 54m 10s ⏱️
 3 869 tests  3 865 ✅   0 💤 4 ❌
72 652 runs  72 545 ✅ 102 💤 5 ❌

For more details on these failures, see this check.

Results for commit f54dc5f.

@vepadulano vepadulano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the comments! Just two last minor suggestions from my side

Comment thread tree/ntuple/test/CMakeLists.txt Outdated
if(pyroot)
ROOT_ADD_PYUNITTEST(ntuple_py_basics ntuple_basics.py)
ROOT_ADD_PYUNITTEST(ntuple_py_model ntuple_model.py)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extra line is not necessary here, please remove it.

" +-----------------------------+\n";
EXPECT_EQ(exp2, os2.str());
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it would be good to reference the original context for the TTree equivalent test. Maybe add a comment just before the class name

This test is a translation using RNTupleProcessor of the test introduced by https://github.com/root-project/root/pull/19322, to ensure that the TTree friendship mechanism works equivalently with the RNTuple join mechanism.

@enirolf enirolf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! I have one minor comment from my side, once that's addressed from my side it's good to go.

Comment on lines +814 to +816
"gh16805_rntuple_friend_0.root",
"gh16805_rntuple_friend_1.root",
"gh16805_rntuple_friend_2.root"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To ensure consistency between terms, I would be in favor to use only "join" when talking about RNTuple, instead of "friends".

Comment thread tree/ntuple/test/ntuple_processor_gh16805.cxx Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants